Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Expose first_device parameter for setting models, scripts, functions #394

Merged
merged 7 commits into from
Oct 12, 2023
Merged

Expose first_device parameter for setting models, scripts, functions #394

merged 7 commits into from
Oct 12, 2023

Conversation

billschereriii
Copy link
Contributor

No description provided.

@billschereriii billschereriii added area: test Issues related to the test suite area: orchestrator Issues related to the Ochestrator API, launch, and runtime user issue Issue posted by user area: api Issues related to API changes labels Oct 12, 2023
@billschereriii billschereriii self-assigned this Oct 12, 2023
al-rigazzi
al-rigazzi reviewed Oct 12, 2023
View reviewed changes
Copy link
Collaborator

@al-rigazzi al-rigazzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good, just a couple minor fixes to make sure all checks pass.

@codecov
Copy link

codecov bot commented Oct 12, 2023 •
edited
Loading

Codecov Report

Merging #394 (ad99dfd) into develop (a9e64c8) will decrease coverage by 0.12%.
The diff coverage is 71.42%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #394      +/-   ##
===========================================
- Coverage    89.77%   89.66%   -0.12%     
===========================================
  Files           59       59              
  Lines         3609     3618       +9     
===========================================
+ Hits          3240     3244       +4     
- Misses         369      374       +5
Files Coverage Δ
smartsim/_core/launcher/colocated.py 97.82% <100.00%> (+0.04%) ⬆️
smartsim/entity/ensemble.py 99.25% <ø> (ø)
smartsim/entity/model.py 95.59% <ø> (ø)
smartsim/entity/dbobject.py 66.08% <66.66%> (-2.44%) ⬇️

al-rigazzi
al-rigazzi approved these changes Oct 12, 2023
View reviewed changes
Copy link
Collaborator

@al-rigazzi al-rigazzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! This addition will empower users on new-gen systems, super-exciting! Thanks for doing this!

MattToast
MattToast requested changes Oct 12, 2023
View reviewed changes
Copy link
Member

@MattToast MattToast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! A couple of pedantic style things, and called out some points where I wanted to check my understanding or bring something to attention, but otherwise looks good to go!

@billschereriii billschereriii added the API break Issues that include incompatible API changes label Oct 12, 2023
MattToast
MattToast approved these changes Oct 12, 2023
View reviewed changes
Copy link
Member

@MattToast MattToast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! One last request for a new test, but otherwise LGTM!

@billschereriii billschereriii merged commit b509efd into CrayLabs:develop Oct 12, 2023
@billschereriii billschereriii deleted the 1dev branch October 12, 2023 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@al-rigazzi al-rigazzi al-rigazzi approved these changes

@MattToast MattToast MattToast approved these changes

Assignees

@billschereriii billschereriii

Labels
API break Issues that include incompatible API changes area: api Issues related to API changes area: orchestrator Issues related to the Ochestrator API, launch, and runtime area: test Issues related to the test suite user issue Issue posted by user
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@billschereriii @al-rigazzi @MattToast